Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

$VIMRUNTIME is already defined in vim #125

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wenzhuoz
Copy link
Contributor

Vim error message in a freshly installed freepbx 17 system:

root@freepbx:~# vim
Error detected while processing /etc/vim/vimrc[60]../etc/vim/vimrc.local:
line    8:
E484: Can't open file /defaults.vim
Press ENTER or type command to continue

Because $VIMRUNTIME is already defined in vim, we don't have to take the trouble to expand it to its actual filesystem path in vimrc.

Copy link

sangoma-oss-cla bot commented Oct 13, 2024

CLA assistant check
All committers have signed the CLA.

@wenzhuoz
Copy link
Contributor Author

The documented method to look up the compiled-in default of $VIMRUNTIME does't work in cloud-init runcmd.

    runcmd:
      - |
        echo -n 'The vim compiled-in default of $VIMRUNTIME: '
        vim -e -T dumb --cmd 'exe "set t_cm=\<C-M>"|echo $VIMRUNTIME|quit' | tr -d '\015'
        echo .

cloud-init-output.log:

The vim compiled-in default of $VIMRUNTIME: .
Cloud-init v. 22.4.2 finished at Mon, 14 Oct 2024 00:09:44 +0000. Datasource DataSourceNoCloud [seed=/var/lib/cloud/seed/nocloud-net][dsmode=net].  Up 33.63 seconds

@push143smart
Copy link
Contributor

Hi @wenzhuoz ,

As outlined in the documentation regarding default of $VIMRUNTIME i.e. method to look up the compiled-in default of $VIMRUNTIME,
it is essential to obtain the value of the VIMRUNTIME variable for use in a shell script.

This may not be applicable in the context of cloud-init.

To address this issue, we can either assign the value to a different variable or verify if the VIMRUNTIME variable is uninitialized. If it is, we can define it and assign the appropriate value; otherwise, we can proceed to use the VIMRUNTIME variable as you have recommended.

Best Regards,
Pushkar

@wenzhuoz
Copy link
Contributor Author

Hi @push143smart ,

Vimrc is interpreted by vim itself and it has a compiled-in default of $VIMRUNTIME, so we don't have to look it up in a script, unless you intend to use a different $VIMRUNTIME folder.

Wenzhuo

@wenzhuoz
Copy link
Contributor Author

wenzhuoz commented Oct 16, 2024

Actually, it's a bad idea to put an absolute path in vimrc, because in a future release upgrade, vim might be upgraded to version 9.1 and the default VIMRUNTIME folder would change to /use/share/vim/vim91, and you'll have to modify vimrc again.

@JoseGoncalves
Copy link
Contributor

JoseGoncalves commented Oct 16, 2024

Hi all. Just to add my feedback on this issue.

I really don't see the point of changing vim configuration in a script that installs FreePBX. I see this as something for advanced users, that should not be part of this script, but that should be adapted pre/post FreePBX installation, if the user requires/needs it.

As an example, in my private fork of this script, I simply removed this vim configuration section, because when I setup a Debian machine I always add the following in my vim custom config at /etc/vim/vimrc.local:

let g:skip_defaults_vim = 1
syntax on
set background=dark
set modeline
set modelines=3
set mouse=

But, once again, I don't think it should be the job of this script to do such kind of initialization...

@wenzhuoz
Copy link
Contributor Author

But, once again, I don't think it should be the job of this script to do such kind of initialization...

Totally agreed.

But if you must do it, just use the variable name $VIMRUNTIME in vimrc instead of the current vim compiled-in default value of it.

@JoseGoncalves
Copy link
Contributor

But, once again, I don't think it should be the job of this script to do such kind of initialization...

While on the issue of what should be on the script or not, another thing that, in my opinion, should not be on the script, is the screen config that precedes the vim config in the script.
The script should have only configs required for the proper FreePBX operation, and should leave things like vim and screen config for advanced users to tune up.

@kguptasangoma
Copy link
Member

Hi @JoseGoncalves I agreed that ideally script should not have dev tools configuration but as this is a basic configuration for editor which just help us to debug better directly on the system we kept in the script.

Otherwise we have to manage other utility to install/configure the system to make debugging easy. We will evaluate this if we see requirement to add more such stuff.

@JoseGoncalves
Copy link
Contributor

Hi @kguptasangoma. One idea, would be to add those dev tools installation & config only if the --dev option that I proposed to introduce in PR #94 was added. If you find that OK, I could adjust the PR accordingly.

@kguptasangoma
Copy link
Member

this is just a basic vim setting , if we come up any more advanced stuff for dev then we could add under dev flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants